-
Notifications
You must be signed in to change notification settings - Fork 36
193 Hide myself from the top hunters and activity lists #199
Conversation
.gitignore
Outdated
@@ -24,3 +24,6 @@ profiles.clj | |||
resources/contracts | |||
node_modules | |||
.DS_Store | |||
src/java | |||
yarn.lock | |||
env/dev/resources/config.edn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When changing that config file, it hangs in git status causing a chance to commit my credentials eventually. The better approach, I believe, would be to provide a sample of that file giving instructions in readme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with the sample file! And we could even separate normal config from sensitive config by using two config files, maybe config.edn
and config-private.edn
, so that we are more aware of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or how about instead of having a sample having a config-default.edn as the sample, which could be imported alongside
config-private.edn`, this one overwriting the custom values from the other one, and this one not being versioned?
Thank you for the PR. Unfortunately, there has been a misunderstanding, as I already have an implementation for this feature. In general, we welcome contributions for issues that have a bounty on them. There are several nice things in this PR not related to the feature that I like (makefile, bug fixes etc) that we certainly want merged. We should figure out how to merge the overlapping changes related to the feature that we now have. I'll get back to this tomorrow. |
Makefile
Outdated
@@ -0,0 +1,41 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the following makefile helps to perform different tasks on the project without forcing you to keep their syntax in mind. For example, to create a new migration, you write: make create-migration
, it promts for its name and creates it as well. Most of modern shells show possible make targets when pressing tab key after make ...
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just using lein aliases so that we avoid new tools and have more power by being able to access our project and language in lein in case we need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, please do not take me wrong, I'm not against lein aliases. There are some advantages of makefile over lein that I'm going to share.
First, starring any lein command takes a REALLY long time. Even without auto-tasks, the delay might be about 5 to 10 seconds. Now Imagine you made a misprint and should call it and wait again.
Then, with auto-tasks specified in lein project, the process of starting repl takes up to minute, because of compiling contracts to Java classes. But I just wanted to start REPL. Would it be better to move compiling process into a separate task? Make target?
Finally, lein utility was created for managing clojure-driven project, but not a common project. For example, in make file you may declare dependencies, saying something like make uberjar
that automatically calls building less, js, contracts, etc tracking their success status.
Makefile is rather a collection of actions that might be performed over the project in general. Say, compiling CSS files is not about clojure, it is more general part. This file is easier to read because of its clear declarative syntax rather then lein project file that usually brings lots of additional stuff.
But still, if you guys do not want to have makefile in the project, there no any claims from my side. I'll just keep it locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, a funny misprint: I'm not against aliases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanations! :)
I'm okay with the makefile as long as it is just to run commands in an easier way, not to develop more logics there.
As for the time that lein takes to run commands and the management those commands, we could probably overcome that with lumo and/or boot to continue having the power of clojure and the integration with the whole project, but that is probably not a priority right now.
project.clj
Outdated
@@ -69,10 +69,8 @@ | |||
[lein-auto "0.1.2"] | |||
[lein-less "1.7.5"] | |||
[lein-shell "0.5.0"] | |||
[cider/cider-nrepl "0.15.0-SNAPSHOT"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since every developer has their own cider version installed, a good practice would be to have in local profiles in ~/.lein/profiles.clj
. For example, I've got 0.15.1 version installed that causes warning messages when connecting to repl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove this change for now? We plan to address that here: #255
ALTER TABLE users | ||
DROP COLUMN is_hidden; | ||
|
||
-- restore the previous version of the view |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here and below, I had to redefine existing DB view to add non-hidden user clause.
src/clj/commiteth/config.clj
Outdated
|
||
(defstate env :start (load-config | ||
:merge | ||
[(args) | ||
(source/from-system-props) | ||
(source/from-env)])) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below, it would be great to allow a developer run exactly those subsystems that he or she really needs when working on a task. Having start!
and stop!
functions helps when you'd like to just start/stop the database for example.
While working on that task, I didn't manage to run some of components due to configuration issues. Instead, I just launched config, db and http-server which were enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about describing the different combinations/groups that could be useful instead of adding a fn to start/stop states independently? In the end starting/stopping states independently can be done easily in the REPL. For example we could have a group to start just the web app without any connection to Ethereum (and without things that depend on that).
src/clj/commiteth/db/core.clj
Outdated
(let [db (env :jdbc-database-url) | ||
migratus-config {:store :database | ||
:migration-dir "migrations/" | ||
:migration-table-name "schema_migrations" | ||
:db db}] | ||
(migratus/migrate migratus-config) | ||
(conman/bind-connection db "sql/queries.sql") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplication line
src/cljs/commiteth/handlers.cljs
Outdated
@@ -320,7 +319,7 @@ | |||
:http {:method POST | |||
:url "/api/user/address" | |||
:on-success #(do | |||
(dispatch [:assoc-in [:user [:address] address]]) | |||
(dispatch [:assoc-in [:user :address] address]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a bug here, when the wrong key was updated so nothing was shown in UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
.gitignore
Outdated
@@ -24,3 +24,6 @@ profiles.clj | |||
resources/contracts | |||
node_modules | |||
.DS_Store | |||
src/java | |||
yarn.lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know we're not using yarn, are we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, we're not, will drop that.
@@ -234,6 +235,14 @@ | |||
(if (= 1 result) | |||
(ok) | |||
(internal-server-error))))) | |||
|
|||
(POST "/hidden" [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using just one endpoint to update data for the user? Before we just had the address, but now this too, and in the future probably more things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, will merge those two handlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, let me ask here
@tpatja where did you update that handler? In your local branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I said in my previous comment, I implemented this whole feature (backend and frontend changes) already last week. My changes are still on my local repo, will clean up and push once I return to work. Will be on holiday until Jan 8th.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that sounds great. But still, I'll refactor that PR to make it satisfy your and @pablanopete suggestions.
2b88442
to
2f24317
Compare
@igrishaev Are you willing to continue and finish this PR? We would like to have it merged. :) We would basically need to only have things related with the hide issue (not additional things, which you can of course propose in different PRs), and to only have one endpoint to send the user profile data. Thanks! :) |
@pablodip yes, sorry for the delay. I wan't sure if should move further with it. Let me adapt it quickly. |
@igrishaev No worries at all, just let us know when it is ready to review it. :) Btw, please also make sure that it is up to date with Thanks a lot! :) |
@siphiuel Good catch! :) Given that we have the |
@pablodip @denis-sharypin Merged with the latest develop and fixed UI. Here how it looks like: |
If heading Settings is there, I'm happy |
@denis-sharypin It is, please update the page (I re-attached the image). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thank you!
@pablodip or anyone else, is there something more required on that task? And for some reason, the issue hasn't been claimed. It's a bit unclear to me how to claim it. Should I send some ETH to the contract address or...? Sorry, it's the first time I contribute to Status. |
The last part of my previous is not action. There was no |
All is good! I've merged it now. This should be claimed by you now in Open Bounty. Again, thank you for your contribution! |
Environments:
Tested functionality:enable/disable hiding Regression:checked that option doesn't affect to Ethereum address field |
Fixes #193
The following PR adds a new option to exclude the current user from the top hunters list and activity feed. Here some screenshots below.
Also, while working on that task, I fixed a couple of bugs and borrowed some features my current Luminus-driven project that help a lot (see my comments below).